Retire the CoverageProvider machinery (PP-4468)#3520
Conversation
|
Claude finished @dbernstein's task in 6m 8s —— View job Code ReviewSummaryThe retirement is well-scoped and the inlined Details
|
8f4574b to
27bf685
Compare
Greptile SummaryThis PR retires the
Confidence Score: 4/5Safe to merge with one concern worth confirming: the exception guard in The removal is well-scoped and all live callers were either migrated or verified as dead code before deletion. The src/palace/manager/integration/license/overdrive/api.py — specifically the exception guard scope in Important Files Changed
|
| try: | ||
| bibliographic.apply( | ||
| self._db, | ||
| edition, | ||
| self.collection, | ||
| replace=ReplacementPolicy.from_license_source(), | ||
| ) | ||
| except Exception as e: | ||
| # Mirror the resilience of the former | ||
| # ``BibliographicCoverageProvider.set_bibliographic``: a database | ||
| # error or unexpected data shape during apply should not crash | ||
| # ``update_licensepool``. Log and leave the pool without updated | ||
| # coverage, matching the prior "silently continue" behavior. | ||
| self.log.warning( | ||
| "Error applying Overdrive bibliographic data to edition %s: %s", | ||
| edition.id, | ||
| e, | ||
| exc_info=e, | ||
| ) | ||
| return |
There was a problem hiding this comment.
metadata_lookup exceptions propagate outside the guard
The except Exception block covers only bibliographic.apply(). A transient HTTP error or malformed JSON response from metadata_lookup() (lines 1812–1813) will propagate to update_licensepool and abort the entire availability update, preventing update_licensepool_with_book_info from running. This matches the old behaviour — the retired process_batch also did not wrap process_item in a try/except — so this is not a regression, but it is worth calling out explicitly because the guard communicates an intent ("bibliographic failures must not crash availability updates") that isn't fully satisfied if the fetch itself throws. Consider wrapping metadata_lookup in the same guard, or adding a dedicated try/except around just that call with a return on failure.
|
@dbernstein it looks like tests are failing on this one, and greptile has a valid review comment about unconditionally calling I'll let you resolve those issues before reviewing this one. When its ready you can request a review from @ThePalaceProject/backend again. |
jonathangreen
left a comment
There was a problem hiding this comment.
Some comments on this for consideration while you are resolving the test issues
| Metadata refresh used to run through the per-source CoverageProvider | ||
| machinery, which has been retired. No provider is wired up, so this | ||
| endpoint is retained for API compatibility but no longer performs a | ||
| refresh; it always reports failure. |
There was a problem hiding this comment.
Since this does nothing can we deprecate it, or just remove it entirely? What calls this endpoint? I think we need a plan to deal with this endpoint since its no longer functional.
There was a problem hiding this comment.
There a 👍🏻 on this, but do we have a ticket or plan for it?
27bf685 to
ac18fb5
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3520 +/- ##
==========================================
+ Coverage 93.46% 93.54% +0.07%
==========================================
Files 512 509 -3
Lines 46611 45964 -647
Branches 6352 6245 -107
==========================================
- Hits 43566 42997 -569
+ Misses 1969 1915 -54
+ Partials 1076 1052 -24 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Apply Overdrive bibliographic metadata directly instead of routing it through the CoverageProvider machinery, then delete the now-dead machinery. Nothing read the coverage rows: the only live caller of ensure_coverage was OverdriveAPI.update_licensepool (force=True, return value ignored), and the admin refresh_metadata endpoint was wired with no provider. The Celery apply task and the Bibliotheca updater already skipped coverage-record writes, so Overdrive was the last writer. - Rewrite OverdriveAPI.update_licensepool to fetch + apply metadata and make the work presentation-ready via a new _ensure_bibliographic_coverage helper, replacing OverdriveBibliographicCoverageProvider.ensure_coverage. - Stop writing coverage records: drop create_coverage_record from BibliographicData.apply and its call sites. - Delete core/coverage.py, the Overdrive and Bibliotheca coverage providers, the coverage-provider runner scripts, Identifier.missing_coverage_from, and the Explain script's coverage block. - Neutralize the now-dead admin refresh_metadata endpoint. - Leave the CoverageRecord model and coveragerecords table in place (dormant) for one release; the drop migration follows in a stacked PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Restore the resilience of the retired BibliographicCoverageProvider: catch and log exceptions from bibliographic.apply in _ensure_bibliographic_coverage so a database error or unexpected data shape during apply does not propagate out of update_licensepool. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address review feedback: the _ensure_bibliographic_coverage docstring, its exception-handling comment, and the matching test comment described the retired CoverageProvider classes instead of what the code does now. Rewrite them to explain the current behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address review feedback: a bare `except Exception` around bibliographic.apply masks programming errors (a mistyped attribute raising AttributeError/KeyError would be silently swallowed). Catch BasePalaceException instead -- apply raises PalaceValueError for invalid data -- so genuine bugs propagate while a single bad title still does not abort the availability update. Update the guard test to raise PalaceValueError and add a test that an unexpected error propagates. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address Greptile feedback: refresh_metadata always returns METADATA_REFRESH_FAILURE, so loading the Work only to discard it was a needless database query on every request. Keep the librarian authorization check and return the failure directly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
test_ensure_bibliographic_coverage_apply_error asserted on caplog.text without setting a capture level, so it relied on the ambient root log level. Under pytest-randomly, a prior test that raises that level leaves the warning uncaptured, so caplog.text is empty and the assertion fails. Set the level explicitly with caplog.set_level(LogLevel.warning), matching the convention used by other caplog tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
e5b1dc0 to
c7d813d
Compare
| try: | ||
| bibliographic.apply( | ||
| self._db, | ||
| edition, | ||
| self.collection, | ||
| replace=ReplacementPolicy.from_license_source(), | ||
| ) | ||
| except BasePalaceException as e: | ||
| # A failure while applying the bibliographic data must not abort the | ||
| # surrounding availability update, so log it and leave the pool's | ||
| # presentation edition unchanged. | ||
| self.log.warning( | ||
| "Error applying Overdrive bibliographic data to edition %s: %s", | ||
| edition.id, | ||
| e, | ||
| exc_info=e, | ||
| ) | ||
| return |
There was a problem hiding this comment.
Exception guard scope excludes SQLAlchemy errors from
apply
The except BasePalaceException block only catches Palace-domain exceptions. BibliographicData.apply performs multiple SQLAlchemy writes (contributors, links, identifiers, equivalencies) that can raise IntegrityError, OperationalError, or DataError — none of which inherit from BasePalaceException. When those slip through, the exception propagates out of _ensure_bibliographic_coverage and into update_licensepool, where there is no further guard, so the outer availability update is aborted for that title.
The old BibliographicCoverageProvider.set_bibliographic wrapped bibliographic.apply in a bare except Exception, which is what the PR description ("broad except Exception") implies was the intent. The two new tests only cover BasePalaceException / KeyError side-effects from a mock, so SQLAlchemy exceptions are untested here.
If the intentional design is to let non-Palace exceptions surface as bugs, the docstring's claim that "a database error … does not propagate out of update_licensepool" should be tightened to match. If the intent truly is to swallow all apply-time errors, the guard should be widened to except Exception.
jonathangreen
left a comment
There was a problem hiding this comment.
Looks like there are some valid AI code review comments that still should be resolved on this one, so I'm going to leave it until you get back from vacation @dbernstein
Description
Retires the legacy
CoverageProvidermachinery. Overdrive bibliographic coverage now runs as a direct metadata-apply step insideOverdriveAPI.update_licensepoolinstead of routing throughOverdriveBibliographicCoverageProvider.ensure_coverage. The baseCoverageProviderclasses, the Overdrive/Bibliotheca providers, the coverage-provider runner scripts, and the now-dead readers (Identifier.missing_coverage_from, theExplainscript's coverage block, the always-failing adminrefresh_metadataprovider path) are deleted.BibliographicData.applyno longer writesCoverageRecordrows (thecreate_coverage_recordparameter is removed; the Celery apply task and the Bibliotheca updater already passedcreate_coverage_record=False).The new
_ensure_bibliographic_coveragehelper wrapsbibliographic.apply(...)in a broadexcept Exceptionthat logs a warning and returns, preserving the resilience of the retiredBibliographicCoverageProvider.set_bibliographicso a database error or unexpected data shape during apply does not propagate out ofupdate_licensepool.The
CoverageRecordmodel andcoveragerecordstable are intentionally left in place (marked dormant) for one release, per the online-migration / N-1 rule. A stacked follow-up PR drops thecoveragerecordsand dormantequivalentscoveragerecordstables.Motivation and Context
JIRA (PP-4468)
The
CoverageProviderbatch-processing pattern is being retired in favor of Celery. Investigation showed nothing live reads coverage rows:ensure_coverage(Overdrive'supdate_licensepool) passesforce=Trueand ignores the result.refresh_metadataendpoint is wired with no provider, so it always returnsMETADATA_REFRESH_FAILUREwithout invoking a provider.Identifier.missing_coverage_from/should_update/ theExplaincoverage block are only reachable through the already-dead runner scripts.The Celery
bibliographic_applytask and the Bibliotheca updater already skip coverage-record writes, so Overdrive was the last writer.How Has This Been Tested?
mypyandruffpass on all changed files.tox -e py312-docker: Overdrive API including the rewrittenupdate_licensepoolbibliographic-coverage path and a new test covering thebibliographic.applyerror guard, the Bibliotheca circulation updater, data-layerapply, and the affected model tests.Checklist
🤖 Generated with Claude Code